-
Notifications
You must be signed in to change notification settings - Fork 119
feat: support retry settings and retryable codes in call context #1238
feat: support retry settings and retryable codes in call context #1238
Conversation
Allows applications to supply retry settings and retryabe codes for individual RPCs through ApiCallContext. Fixes googleapis#1197
Codecov Report
@@ Coverage Diff @@
## master #1238 +/- ##
============================================
- Coverage 79.65% 79.49% -0.16%
- Complexity 1249 1291 +42
============================================
Files 209 215 +6
Lines 5461 5516 +55
Branches 464 472 +8
============================================
+ Hits 4350 4385 +35
- Misses 928 945 +17
- Partials 183 186 +3
Continue to review full report at Codecov.
|
@igorbernstein2 Friendly ping on this PR. Would you mind taking a look? If you're not the best person to weigh in on this, then please let me know. cc @skuruppu |
This looks great (to my untrained Java eye). I'd like @igorbernstein2 and/or @vam-google / @miraleung to review it though, they are the more Java savvy and experienced in this codebase. Of course, @chingor13 should be approve of the added functionality (I already polled these folks prior to starting this work, but just for posterity...). |
this.channel = channel; | ||
this.callOptions = Preconditions.checkNotNull(callOptions); | ||
this.timeout = timeout; | ||
this.streamWaitTimeout = streamWaitTimeout; | ||
this.streamIdleTimeout = streamIdleTimeout; | ||
this.channelAffinity = channelAffinity; | ||
this.extraHeaders = Preconditions.checkNotNull(extraHeaders); | ||
this.retrySettings = retrySettings; | ||
this.retryableCodes = retryableCodes == null ? null : ImmutableSet.copyOf(retryableCodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this nullable? per Effective Java, empty collections are preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference in this case:
null
means 'do not override the default settings`.- An empty set means 'override the default settings by disabling retrying'.
I've added that to the documentation of ApiCallContext#getRetryableCodes()
.
gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/ApiResultRetryAlgorithmTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/ApiResultRetryAlgorithmTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/ApiResultRetryAlgorithmTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do the docs for retries live? Does something need to be added to package-info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google policy is that existing violations of the style guide do not justify further violations. All new code must follow the style guide, irrespective of existing code.
@elharo Thanks for the review. I (think I) have addressed all your comments. PTAL. |
@elharo Friendly ping on this PR. I addressed your comments, would you mind taking a second look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with few (hopefully last ones) comments. Basically I still hope we can get rid of the Ignore*
classes.
Also, optionally, please consider having some clear strategy for all of the context-aware and context-unaware methods:
- Make context-unaware methods deprecated,
- make sure that one implementation always calls another one to avoid code duplication (if context-unaware implementaitons got deprecated then it should be context-unaware call context-aware, with context probably as
null
) - make sure that methods documentation reference each other and explains the situation (that both methods are the same thing, and context-awareness is the only difference)
* RetryingContext}. This is used to wrap {@link ResultRetryAlgorithm} instances to create a {@link | ||
* ResultRetryAlgorithmWithContext} when one is required. | ||
*/ | ||
class IgnoreRetryingContextResultRetryAlgorithm<ResponseT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way the new constructor is implemented in RetryAlgorithm
(the one which implementation you listed in the comment) seems being kind of against OOP principles, since the constructor does accept a generic interface, but then instantiates the specific instances of the interface instead. I understand that sometimes we may need to do something like this, but I'm not sure it is that case.
Looks like the Ignore*
classes exist only because RetryAlgorithm
implementation changed the aggregated fields type to the context-aware interfaces:
private final ResultRetryAlgorithmWithContext<ResponseT> resultAlgorithm;
private final TimedRetryAlgorithmWithContext timedAlgorithm;
A the same time this PR adds the full copy of all the existing methods in RetryAlgorithm but with the context extra parameter now. I.e. this means that this class can be used eitehr as context-aware one or as context-unaware, but not in both different context. WDYT about instead of adding the new Ignore*
classes, just make RetryAlgorithm just have both types of algorithms aggregated:
private final ResultRetryAlgorithm<ResponseT> resultAlgorithm;
private final TimedRetryAlgorithm timedAlgorithm;
private final ResultRetryAlgorithmWithContext<ResponseT> resultAlgorithmWithContext;
private final TimedRetryAlgorithmWithContext timedAlgorithmWithContext;
And use each of the algorithms accordingly depending in which method of REtryAlgorithm class it is used (context-aware or not).
In general, I'm just trying to avoid having classes like IgnoreRetryingContextReulstRetryAlgorithm
because they are obviously a hack (even check the name, it is too long an unreadable). So here I'm trying to trade it to simply having context-aware fields as null
for older constructor, and it is ok to throw an exception if context-aware methods are used for RetryAlgorithm initialized in legacy way.
Also, as you suggest in your comments, please go ahead and deprecate the old constructor for RetryAlgorithm
and relevant stuff (maybe even the old context-unaware methods as well).
* @param previousResponse response returned by the previous attempt | ||
* @param previousSettings previous attempt settings | ||
*/ | ||
public TimedAttemptSettings createNextAttempt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use access modifiers (public
) on interface methods, as they are redundant.
Throwable previousThrowable, | ||
ResponseT previousResponse, | ||
TimedAttemptSettings previousSettings) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you call createNextAttempt()
(no context one) here for consistency with the shouldRetry
implementation?
extends ResultRetryAlgorithm<ResponseT> { | ||
|
||
/** | ||
* Creates a next attempt {@link TimedAttemptSettings}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional (but please consider doing it). It seems like a lot of method duplication is happening here (context-aware vs context-unaware methods). Please consider replacing documentation for every context-unaware method in all of the classes with a link to the context-aware one with the explanation, that this one (context-unaware one) is doing the same but without the context. This will not only can make it shorter in terms of docs, but will also provide a clear picture to the users of these classes why there are both types of methods (which may be really confusing).
Also, please consider deprecating the context-unaware methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I've updated the documentation of the context-unaware interfaces and their methods to reference the context-aware versions.
- I've deprecated all the context-unaware methods
* <p>The attempt settings will be reset if the stream attempt produced any messages. | ||
*/ | ||
@Override | ||
public TimedAttemptSettings createNextAttempt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these methods (context-aware and context-unaware methods in streaming algorithm) call each other, like it is done in the non-streaming versions? This is to reduce code dupliation and make the implementation more robust in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (this is now possible as the context-aware methods accept null values for the context)
* StatusCode.Code.DEADLINE_EXCEEDED)); | ||
* }</pre> | ||
*/ | ||
ApiCallContext withRetrySettings(RetrySettings retrySettings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark these new methods as BetaApi
. This an addition to a fundamental interface, whatever we add here we will have to stick with for a really long time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…a into context-retry-settings
…a into context-retry-settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please cleanup the deprecation stuff (more details in the comment). Sorry for confusing with it.
* thrown instead | ||
* @param nextAttemptSettings attempt settings, which will be used for the next attempt, if | ||
* accepted | ||
* @throws CancellationException if the retrying process should be canceled | ||
* @return {@code true} if another attempt should be made, or {@code false} otherwise | ||
*/ | ||
public boolean shouldRetry( | ||
Throwable prevThrowable, ResponseT prevResponse, TimedAttemptSettings nextAttemptSettings) | ||
Throwable previousThrowable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for confusing with my @Deprecate
request. I think we should just mark the methods and constructors in RetryAlgorithm
class as deprecated (the old ones) and make sure that the non-deprecated methos (new ones) are called by the deprecated methods (which is what is happening now, so it is all good), otherwise we will be using our own deprecated methods to implement non-deprecated methods, which is wrong.
For the rest of it, please undeprecate the methods back, as is is clear that RetryAlgorithm
cannot be used in both contexts (as context-aware and as unaware), but the actual interfaces (timed and result) kind of still potentially can have meaningful implementation for all the methods, but most importantly it is too much deprecation happening on interface level, I'm afraid we are going to get too many deprecaton warnings.
So to summarize: deprecate the old constructor and methods in RetryAlgorithm
class, undeprecate the other stuff (the TimedRetryAlgorithm and ResultRetryAlgorithm methods - in both places, the @Deprecated
annotation and @deprecated
java doc element, maybe adding in the general java doc section of each element a notion that the other method should be preferred, but it does not mean it is deprecated in a strict sense of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@olavloite heads up #1328 we are wrapping all RPC callables in the retry machinery which should enable the use of this feature on non-retryable RPCs once merged. Note: #1328 has the same numbers as this PR 1238, how about that! :) |
@miraleung can you review/approve this PR again? You requested changes at one point. |
Cool (both the feature and the numbers) :-) |
BTW @olavloite if this is good to go, feel free to merge this - no need to wait on the callable wrapping change I mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please wait for others to LGTM as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM this is really awesome, Knut
Removed @igorbernstein2 because we had thorough reviews from others. |
Adds support for retry settings and retryable codes for individual RPCs through
ApiCallContext
.Example usage:
Points for consideration:
gax-java/gax/src/main/java/com/google/api/gax/rpc/Callables.java
Line 86 in aabd3b0
withRetrySettings(...)
orwithRetryableCodes(...)
will not change the retry behavior of those RPC's.ContextAware...
classes that accept aRetryingContext
as an argument for the methods that determine whether to retry an RPC, and if so, with what settings. This ensures that this PR is not a breaking change. It would also be possible to add this behavior to existing classes, but that would mean:2.1. It would be a breaking change
2.2. OR it would include multiple
if (someAlgorithm instanceof ContextAwareAlgorithm) return ((ContextAwareAlgorithm) algorithm).shouldRetry(context, ...);
type statements in the existing classes to check whether it has been passed an algorithm that is able to determine retry settings using aRetryingContext
or not.Fixes #1197